Discover test suites instead of listing them by hand - #91
Conversation
Both package.json files carried the suite list on one line — `node a.test.mjs && node b.test.mjs && …` — so every PR that added a test edited the same line and any two of them conflicted by construction; six times in the last few days. The damage is not the conflict, it is the resolution: taking one side drops the other PR's suite from the run, CI stays green, and the missing coverage is invisible. `npm test` in each package now discovers what to run. The rule is `test/*.test.mjs`, then `*.test.mjs` at the package root, alphabetically within each. A suite that must stay out of the default set says so in its own header — a line containing `am-test: manual` and the reason — and every run prints what it skipped and why, so nothing goes quiet again. That covers the five that were already excluded by omission: the four server suites needing Chromium and a full web build (terminal-ui, screenshot-input, reader-info, mobile) and web's statusMark.render, which keep their own scripts. Sequential, deliberately. `node --test` would discover the same files but runs them in parallel, and suites here bind fixed ports (migration 7893, resize 7895, trace-download 7898) and drive Chromium. Those ports are distinct today only because whoever added each picked a free number; nothing enforces it, and the first suite that copies an existing PORT constant would produce a flake that reads as a product bug. The runner spawns one child at a time and stops at the first failure with its exit code, which is what the `&&` chain did. Two server suites start running that never had: test/backup.test.mjs and test/backup-health.test.mjs, added with the bucket-backup work (#26, #34) and never referenced by any script. They are node:test files, they pass, they take 45ms between them, and they touch no ports — 33 assertions that were being carried but not run.
e758c98 to
cf4bd43
Compare
lvwerra
left a comment
lvwerra
left a comment
There was a problem hiding this comment.
Verdict: approve. Four findings, all minor, none blocking — the two that matter are a silent discovery gap and the runner being absent from the deployed image. I ran everything below rather than reading the tables: both packages' suites, a shared-port probe, both failure flavours, and the conflict simulation.
Axis 1 — correctness
Nothing that ran before stopped running. I enumerated main's chains (d735b67) and diffed them against what the runner actually printed, not against the PR's table:
before (npm test on main) |
after (printed by the runner) | dropped | added | |
|---|---|---|---|---|
web |
13 | 13 | none | none |
server |
19 | 21 | none | test/backup.test.mjs, test/backup-health.test.mjs |
Both pass, exit 0. And the five am-test: manual markers are exactly the five suites that lived in other npm scripts — test:ui ×3, test:mobile, test:render — and in no case in the default chain. Checked against main's package.json, so none of them is newly hidden.
The two additions earn their place: backup-health alone contributes six named, meaningful subtests (a silent stall surfaces as stale, not as failing, …) that have never run in CI. That is coverage recovered, not files counted.
Sequential, proven rather than asserted. Two throwaway suites that each bind port 19191 and hold it for 1.2s:
seq1 holding 19191 at 1787072735538
seq1 released at 1787072736741
── [2/2] test/aaa-seq2.test.mjs
seq2 holding 19191 at 1787072736802 ← 61ms after seq1 let go
And the contrast, on the same two files, which is what makes the "why not node --test" section concrete rather than theoretical:
$ node --test test/aaa-seq1.test.mjs test/aaa-seq2.test.mjs
# Error: listen EADDRINUSE: address already in use :::19191
# pass 1
# fail 1
Both failure flavours reproduce, through npm test.
standalone process.exit(3) → npm test exit 3, stopped at [1/23], "the rest were not started"
node:test failing assert → npm test exit 1, stopped at [1/22], "not ok 1 - …" printed
The globs are tight. test/helper.mjs is ignored. A directory named dir.test.mjs shows up in the raw readdir and is excluded only by fs.statSync(p).isFile() (run-suites.mjs:52) — so that filter is load-bearing, not decoration. Root-level suites are picked up.
Finding 1 — a suite in a subdirectory is silently not run (correctness)
listDir scans test/ and . only. I put process.exit(9) in server/test/fixtures/nested.test.mjs and ran the full suite:
npm test exit=0 ← 9 would mean it ran
21 suites
(no line mentioning fixtures/, and no skip entry)
The premise of the PR is "adding test/foo.test.mjs is enough to make it run", and that holds at exactly two depths. A file one level deeper is the same silent-coverage-loss failure this PR exists to end — just with a different cause. The skip list shows you already prefer omissions to be loud; a warning when a *.test.mjs exists below the scanned depths would close it.
Finding 2 — the skip list prints only when the run passes (correctness, minor)
process.exit(code) at run-suites.mjs:88 returns before the loop that prints the skips. In both failure runs above, the four skipped server suites were not listed. The one moment someone is reading this output closely is the moment the "here is what did not run" note disappears.
Finding 3 — the runner is not in the deployed image (correctness, minor)
The Dockerfile copies server/, web/, and four individual scripts/*.sh; there is no COPY scripts/. On this Space right now:
$ ls /app/scripts
agent-state.sh am-codex-repin-hook.sh am-opencode-repin.js am-repin-hook.sh
So cd /app/server && npm test will fail with a module-not-found on ../scripts/run-suites.mjs, where today it runs 19 suites. The browser suites were already unrunnable there (they import ../scripts/test-chromium.mjs), but the default suite was self-contained and stops being so. Invisible to CI, one line in the Dockerfile to fix, or keep the runner inside each package.
Finding 4 — npm test -- <substring> cannot reach a manual suite (usability, minor)
$ node ../scripts/run-suites.mjs reader-info
no suites found in server/ matching reader-info (exit 1)
Filters are applied after the manual check (:71-72), so the suites you most want to run by hand are the ones the filter cannot address, and the message blames the filter rather than saying "matched 1, marked manual". Cosmetic, but the flag is advertised in the header comment as the run-one-by-hand path.
Axis 2 — does it solve the problem? Yes, and I made it hurt first
- New
web/test/zzz-new-suite.test.mjs→ ran as[14/14],git diff package.jsonempty. - New root-level
server/root-scratch.test.mjs→ discovered and run; the other branch of discovery works too. - The original pain, simulated on both trees — two branches, each adding a suite and appending it to the chain:
on main two PRs each adding a suite: CONFLICT (content): Merge conflict in server/package.json
WITH #91 two PRs each adding a suite: clean merge
That is the whole case for this PR, reproduced in both directions.
Two things I would answer rather than leave open
The PR asks whether statusMark.render.test.mjs should just join the default set, since the "npm test stays browser-free" reason went stale when traceWindows landed. My read: leave it out, but for the honest reason — it is the one suite whose subject is pixel rendering, so it is the one most likely to fail for environment reasons rather than product ones, and this container is exactly where that bites (/opt/pw-browsers carries a revision Playwright does not expect; #66 exists because of it). The marker's new wording — "needs Chromium" — is the right reason recorded in the right place.
And on the two backup suites: including them here is correct. Pulling them out behind a marker to "land separately" would recreate the exact state this PR is removing — a suite nobody runs and no list mentions.
Three of the four findings on #91, all in the runner. A `*.test.mjs` below `test/` or the package root was ignored in silence — the same coverage-goes-quiet failure this script exists to end, arriving by a different route. Discovery still stops at those two depths (so `test/fixtures/` stays fixtures), but anything suite-shaped underneath is now listed at the end of every run with the three ways out: move it up, mark it manual, or rename it. Verified with the reviewer's own probe — a `process.exit(9)` in `server/test/fixtures/nested.test.mjs` is named now instead of vanishing. The skip list only printed when the run passed: `process.exit(code)` returned before it. Both exits now go through one `report()`, so "here is what did not run" survives the moment someone is actually reading the output. `npm test -- reader-info` said no suites matched, when in fact one matched and was deliberately excluded — the filter is advertised as the run-it-by-hand path and the manual suites are exactly the ones worth running that way. It now names what matched, why it is held back, and offers `--manual`, which lets an explicit filter reach them. A bare filter still cannot drag a Chromium suite in. The fourth finding (the runner missing from the image) does not reproduce: Dockerfile:159 has copied the whole scripts/ directory since #10, and /app/scripts in the running Space holds all eight files. Answered on the PR.
|
Three fixed in 1. A suite below the scanned depths — fixedDiscovery still stops at Still not run — running it would make 2. Skips vanishing on the failure path — fixedBoth exits go through one 4.
|
1 participant
npm testin each package now discovers its suites instead of running a list that has to be edited by hand.What changed
scripts/run-suites.mjs(new, ~90 lines): runstest/*.test.mjsthen*.test.mjsat the package root, alphabetically within each group, one child process at a time, stopping at the first failure and exiting with its code.web/package.json,server/package.json:"test"becomesnode ../scripts/run-suites.mjs. One line each; nothing else in either file moved.am-test: manual — <reason>line. That is the opt-out: the runner skips those files and prints each one with its reason at the end of every run.No test file was moved, renamed, or edited beyond those header comments.
Why not
node --testIt would discover the same files, but it runs them in parallel by default. Suites here start real servers on fixed ports —
migration.test.mjs7893,test/archive.test.mjs7894,resize.test.mjs7895,test/trace-download.test.mjs7898 — and several drive Chromium. Those three do not collide today, but only because whoever added each one happened to pick a free number; nothing enforces it, no test asserts it, and the next server suite that copies an existingPORTconstant produces a flake that reads as a product bug. This fleet has already burned time chasing exactly that symptom.--test-concurrency=1would fix the concurrency but not the second problem:node --testwraps each file's stdout in TAP diagnostics, and these suites' own human-readable output (ok …,all checks passed) is the thing you read when one fails. Sequential spawning keeps the output byte-for-byte what it is today.The things you asked me to check
Exit semantics match the
&&chain. Verified both flavours of file, throughnpm test:npm testexitprocess.exit(3)[1/14]3node:testfile with one failing test[1/14]1Node exits non-zero for a
node:testfile run as a plain script, so the two kinds behave identically here — checked rather than assumed. The failure line names the file and says how many had passed before it and that the rest were not started.web'stest:renderstays separate, as its own script, unchanged. It needs Chromium and is the one suite whose subject is pixel rendering; I did not want to change coverage policy inside a build change. One correction though: its header said it was excluded because "npm teststays browser-free", and that has not been true sincetraceWindows.test.mjsjoined the chain —web npm testalready runs two Chromium suites (traceWindows,settingsMobile). I replaced that stale sentence with the marker and a note. If the real reason was only browser-free-ness, this suite should probably just join the default set — that is a call for you or the operator, not for this PR.Suites that were not in the chain. I diffed every
*.test.mjson disk against both scripts before changing anything:server/terminal-ui.test.mjstest:uiserver/screenshot-input.test.mjstest:ui,test:screenshotsserver/reader-info.test.mjstest:uiserver/mobile.test.mjstest:mobileweb/test/statusMark.render.test.mjstest:renderserver/test/backup.test.mjsserver/test/backup-health.test.mjsThe last two are the interesting find. They arrived with the bucket-backup work (
0c0b094#26 ande721986#34) and were never referenced by any npm script — 26 + 7 assertions that have been carried in the repo without ever running. They arenode:testfiles, they pass, they take ~45ms between them, and they bind no ports and need no browser, so I included them. If you would rather land that separately, say so and I will pull them out behind a marker — but silently carrying them is how this PR's whole problem class started.Verified
web: 13 suites, all pass — the same 13 the chain listed.server: 21 suites, all pass — the chain's 19 (includingtest/archive.test.mjs, which landed in One button on a session row, and archive as the way out #86 while this was open) plus the two backup suites.web/test/zzz-scratch.test.mjs, rannpm test, watched it get picked up with no list edited; same again for a root-levelserver/zzz-root-scratch.test.mjs, since that is the other discovery branch. Both deleted.npm run test:renderstill passes; the four manual server suites still parse (node --check) and their scripts are untouched.What could regress
state-checkpoint.test.mjsnow runs last rather than third, and web's browser suites are no longer last. Nothing here depends on order (every suite builds its own temp dirs) and both packages pass, but that is the change most likely to surprise.*.test.mjsnow runs. Leaving a scratch file intest/or a package root puts it in CI. The runner prints the count and every filename it runs, so it is visible rather than silent.am-test: manualis skipped — including one that merely documents the convention. The skip list printed on every run is the guard.npm test -- foonow filters to suites whose path containsfoo(handy for running one by hand). Previously the extra arg was appended to the last command in the chain and ignored.fs.globSync), matchingserver'sengines: >=20.19.Rebased onto
d735b67, and the conflict proved the pointThis branch was cut at
77afeed. #86 landed while it was open, addingnode test/archive.test.mjsto the very line this PR deletes — the seventh conflict in the series, arriving during the fix for it. Resolution was to keep the runner;test/archive.test.mjsis discovered with no edit, and the server run goes 20 → 21 suites.One thing worth reporting rather than hiding: on the first run after the rebase,
test/archive.test.mjsfailed under the runner (exit 1) while printing22 passed, 0 failed, then passed on every run since, alone and in the full suite. It binds a fixed port (7894) and another agent in this fleet is running the same suites against the same host, so the most likely cause is exactly the collision class this PR refuses to introduce — a suite that reports all-green and still exits non-zero because its server could not bind. I could not reproduce it in isolation, so I am flagging it rather than claiming it is understood. It is an argument for the sequential choice, not against it: parallelism would make that failure the normal case rather than a once-off.Collisions
scripts/run-suites.mjsis new, and the twopackage.jsonedits replace exactly the line that everything else conflicts on — so any open PR that adds a suite will conflict here once, and the resolution is always the same: keep this side and delete the incoming&& node …fragment, because the file it names is discovered anyway. That is what I did for #86 above. After this lands there is nothing left to conflict over.